Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update getPullRequestsMergedBetween to run pre-set string command #4336

Merged
merged 3 commits into from
Aug 6, 2021
Merged

Update getPullRequestsMergedBetween to run pre-set string command #4336

merged 3 commits into from
Aug 6, 2021

Conversation

Jag96
Copy link
Contributor

@Jag96 Jag96 commented Jul 30, 2021 •
edited
Loading

@roryabraham will you please review this
cc @AndrewGable

Details

This PR attempts to fix an issue where when a CP happens, the toRef is set to an empty string, even though it is being logged out properly beforehand. Came up with this approach after looking at this SO and seeing that our other usages of execSync only set variables that are separated by spaces, so perhaps there is an issue in this case. I also added a log line that will spit out the command to ensure that the string is what we expect it to be.

Fixed Issues

Related to https://github.com/Expensify/Expensify/issues/168349, possibly a fix 🤞

Tests

  1. Merge this PR
  2. With a staging deploy checklist open, CP a new PR
  3. Confirm that the deploy checklist issue description isn't updated with PRs that aren't on staging

I ran tests locally to confirm that the function still works as expected:

image

QA Steps

None

Screenshots

N/A

@Jag96 Jag96 requested a review from roryabraham July 30, 2021 20:25
@Jag96 Jag96 self-assigned this Jul 30, 2021
@Jag96 Jag96 requested a review from a team as a code owner July 30, 2021 20:25
@MelvinBot MelvinBot requested review from aldo-expensify and removed request for a team July 30, 2021 20:26
@aldo-expensify
Copy link
Contributor

@Jag96 from what I see, and I think this what you wanted to achieve, this is logging some extra stuff and not changing the return of the function. But unfortunately due to my lack of experience in the area, I don't know how to test this! Feel free to remove me and find someone else that can give you a better input about this if you need :)

roryabraham
roryabraham approved these changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely worth a shot! Let's test this on the next checklist by CPing it as soon as the checklist is locked.

@OSBotify
Copy link
Contributor

OSBotify commented Aug 2, 2021

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@roryabraham
Copy link
Contributor

cc @isagoico because I know you'll be more stoked about this than anyone if it works.

aldo-expensify
aldo-expensify approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the test local test and confirmed that it is working. I'll let the merge to you guys because I'm not familiarized enough with the process to test it in staging.

@Jag96
Copy link
Contributor Author

Jag96 commented Aug 5, 2021

For context, we're holding off on merging this until we have a fresh checklist to confirm the issue is fixed. Putting this on HOLD for now

@Jag96 Jag96 changed the title Update getPullRequestsMergedBetween to run pre-set string command [HOLD for fresh Deploy Checklist] Update getPullRequestsMergedBetween to run pre-set string command Aug 5, 2021
@Jag96 Jag96 changed the title [HOLD for fresh Deploy Checklist] Update getPullRequestsMergedBetween to run pre-set string command Update getPullRequestsMergedBetween to run pre-set string command Aug 6, 2021
@Jag96
Copy link
Contributor Author

Jag96 commented Aug 6, 2021

Going to CP this to Staging before the prod deploy

@AndrewGable AndrewGable merged commit 27e9f06 into main Aug 6, 2021
@AndrewGable AndrewGable deleted the joe-fix-cp-bug branch August 6, 2021 02:01
github-actions bot pushed a commit that referenced this pull request Aug 6, 2021
@AndrewGable
Merge pull request #4336 from Expensify/joe-fix-cp-bug
ab91969 
Update getPullRequestsMergedBetween to run pre-set string command

(cherry picked from commit 27e9f06)
This was referenced Aug 6, 2021
Merged
Closed
Closed
@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to production in version: 1.0.82-7🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-7🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify OSBotify mentioned this pull request Aug 11, 2021
Closed
53 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify OSBotify mentioned this pull request Aug 17, 2021